Skip to content

Re-enable abs_integrate for Maxima integration #40574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Re-enable abs_integrate for Maxima integration #40574

wants to merge 7 commits into from

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Aug 11, 2025 •
edited
Loading

We used to have Maxima's abs_integrate package loaded by default, but it caused more problems than it solved, and was disabled in #12731. All known issues have been resolved, however:

This PR cleans up some existing tests, debt left over from when abs_integrate was disabled, and then re-enables it with a whole new collection of tests for the old problems. We do not gain as much as we used to (vanilla maxima has improved), but we do get some extra integrals for "free" now that the problems appear to be resolved.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 11, 2025 •
edited
Loading

Documentation preview for this PR (built with commit e663349; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@dimpase
Copy link
Member

dimpase commented Aug 12, 2025

one can load Maxima's abs_integrate explicitly in tests which need it; I'm -1 on just removing these and sweeping the abs_integrate problems under the carpet this way.

@orlitzky
Copy link
Contributor Author

The problem was that it causes some (new) incorrect results, and you can't unload it.

Several of these tests now work without abs_integrate. The # known bug examples that are removed are,

  • integrate(1/(1 + abs(x)), x) does not get evaluated
  • integrate(cos(x + abs(x)), x) does not get evaluated
  • integrate(e^(-x^2/2)/sqrt(2*pi) * sgn(x-1), x, -Infinity, Infinity) evaluates to something uglier than expected
  • integrate(sqrt(x + sqrt(x)), x).canonicalize_radical() evaluates to something uglier than expected

The last two may be unattractive, but you do get an answer, so it's only the first two where we completely lose the ability to integrate them. And the first is covered by the maxima documentation: https://maxima.sourceforge.io/docs/manual/de/maxima_181.html.

What do we gain by testing them and then throwing away the result?

@dimpase
Copy link
Member

dimpase commented Aug 12, 2025

At least, leave a comment somewhere on the use of abs_integrate

@orlitzky orlitzky force-pushed the abs_integrate-docs branch from 03e2d9c to 4d02f7f Compare August 13, 2025 00:25
@orlitzky
Copy link
Contributor Author

Ok, the new commit adds back the two examples where maxima is helpless without abs_integrate, and provides a much better explanation of the situation (including instructions for loading abs_integrate). We now test that the integrals are not solved, so if that ever changes we will know.

@orlitzky
Copy link
Contributor Author

It actually looks like many of the abs_integrate problems we had should be fixed (upstream). Some day I may try re-enabling it, but the number of integrals that require it has also gone down, so there is a lot less motivation to rock the boat.

@orlitzky orlitzky force-pushed the abs_integrate-docs branch from c454002 to 1a69eea Compare August 13, 2025 17:23
@orlitzky
Copy link
Contributor Author

Added an experimental commit to re-enable abs_integrate (with tests) to see what happens.

@dimpase
Copy link
Member

dimpase commented Aug 13, 2025 via email

@orlitzky
Copy link
Contributor Author

5.48

@dimpase
Copy link
Member

dimpase commented Aug 13, 2025

we should get sage to 5.48 too

@orlitzky orlitzky changed the title Clean up some old abs_integrate tests Update to maxima-5.48.0 and re-enable abs_integrate Aug 14, 2025
@orlitzky
Copy link
Contributor Author

The tests pass, so maybe let's keep it enabled? I upgraded maxima as well.

@cxzhong
Copy link
Contributor

cxzhong commented Aug 15, 2025

I just check the website 5.48.1 is out. I think we can directly bump to 5.48.1 which has more bug fixes @orlitzky

@orlitzky orlitzky force-pushed the abs_integrate-docs branch from 9ac0c0a to f7a1a90 Compare August 15, 2025 13:36
@cxzhong
Copy link
Contributor

cxzhong commented Aug 15, 2025

And you need to change the title @orlitzky 5.48.0 to 5.48.1

@orlitzky orlitzky changed the title Update to maxima-5.48.0 and re-enable abs_integrate Update to maxima-5.48.1 and re-enable abs_integrate Aug 15, 2025
@orlitzky orlitzky force-pushed the abs_integrate-docs branch from 16a04e2 to beb5b75 Compare August 16, 2025 15:54
@orlitzky orlitzky changed the title Update to maxima-5.48.1 and re-enable abs_integrate Update to maxima-5.48.0 and re-enable abs_integrate Aug 16, 2025
@orlitzky
Copy link
Contributor Author

I put this back to 5.48.0 for now. Some test output has changed in v5.48.1 that warrants further scrutiny before updating the expected output.

@orlitzky
build/pkgs/maxima/SPKG.rst: delete old patch explanations
06bd5d7 
We are still describing a bunch of patches that don't exist.
@orlitzky
src/sage/interfaces/maxima_lib.py: update some integration doctests
7750c57 
There are some tests in this file that still mention abs_integrate,
though it was disabled a few years ago. We remove all mention of it,
and clean up a bit:

  * Drop a test repeated in src/sage/symbolic/integration/integral.py
  * Remove "# known bug" from any examples that now pass without
    abs_integrate
  * Delete any remaining "# known bug" tests. They are not really bugs
    -- you just don't get a nice answer -- and it is a waste of time to
    run them and then throw away the answer.
@orlitzky
src/sage/interfaces/maxima_lib.py: add back two abs_integrate examples
db6ada4 
Replace two examples that can only be solved when abs_integrate is
loaded. These examples are now tested to NOT produce a meaningful
result, so that if in the future they can be solved without
abs_integrate, we will know. The surrounding text explains why
the abs_integrate package is not loaded by default, and how to
load it manually.
@orlitzky
src/sage/interfaces/maxima_lib.py: re-enable abs_integrate
32c180a 
All of the issues we encountered (and reported) with abs_integrate
have been fixed. We re-enable it with doctests to,

  1. Ensure that none of those errors have returned
  2. Test that we can solve some new integrals

The list of problems comes from,

  sagemath#12731

With the upstream fixes from,

  https://sourceforge.net/p/maxima/code/ci/3ca4235b
@orlitzky
src/sage/interfaces/maxima_lib.py: disable one abs_integrate test
dc9ebd7 
The wrong answer is returned unless you have Maxima 5.48, which is
not widespread enough for us to require it yet.
@orlitzky
src/sage/calculus/desolvers.py: update test output
ce0c480 
We can solve one additional integral in this file now that
abs_integrate is back.
@orlitzky
src/sage/functions/piecewise.py: update test output
e663349 
We can solve one additional integral in this file now that
abs_integrate is back.
@orlitzky orlitzky force-pushed the abs_integrate-docs branch from beb5b75 to 458b006 Compare August 16, 2025 16:00
@orlitzky orlitzky force-pushed the abs_integrate-docs branch from 458b006 to e663349 Compare August 16, 2025 19:24
@orlitzky orlitzky changed the title Update to maxima-5.48.0 and re-enable abs_integrate Re-enable abs_integrate for Maxima integration Aug 16, 2025
@orlitzky
Copy link
Contributor Author

Maybe the failures were there in 5.48.0 too and I wasn't paying attention? IDK, back to 5.47.0 to see what happens.

@orlitzky
Copy link
Contributor Author

It's OK with 5.47.0. I'd prefer to put off the upgrade until I don't have so many other things going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
s: needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@orlitzky @dimpase @cxzhong